Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add exclusions mechanism into jwtproxy config builder & exlude liveness probes from auth #10463

Merged
merged 11 commits into from
Jul 24, 2018

Conversation

mshaposhnik
Copy link
Contributor

@mshaposhnik mshaposhnik commented Jul 18, 2018

What does this PR do?

  • Adds code that parse exclusions in the server configuration attributes and puts it into jwtproxy conf.
  • Adds exec, terminal and wsagent http server /liveness paths into excludes;
  • Adds some jwtproxy required JWT claims like IAT, NBF, etc

What issues does this PR fix or reference?

#10400

Release Notes

N/A

Docs PR

N/A

@mshaposhnik
Copy link
Contributor Author

ci-test

@benoitf benoitf added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. kind/task Internal things, technical debt, and to-do tasks to be performed. labels Jul 18, 2018
@@ -77,6 +81,7 @@

static final String JWT_PROXY_CONFIG_FOLDER = "/config";
static final String JWT_PROXY_PUBLIC_KEY_FILE = "mykey.pub";
static final String UNSECURED_PATHS_ATTRIBUTE = "unsecuredPaths";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@riuvshin
Copy link
Contributor

ci-test build report:
Build details
Test report
selenium tests report data
docker image: eclipseche/che-server:10463
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@@ -11,15 +11,17 @@
"protocol": "http",
"path" : "/process",
"attributes": {
"secure": "true"
"secure": "true",
"unsecuredPaths" : "/liveness"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove space before :

@mshaposhnik
Copy link
Contributor Author

ci-test

Copy link
Member

@sleshchenko sleshchenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

public_key_path: /config/mykey.pub
claims_verifiers:
- type: static
options:
iss: wsmaster
nonce_storage:
type: void
excludes:

This comment was marked as resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

@riuvshin
Copy link
Contributor

ci-test build report:
Build details
Test report
selenium tests report data
docker image: eclipseche/che-server:10463
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@mshaposhnik mshaposhnik requested a review from a user July 18, 2018 14:39
@mshaposhnik
Copy link
Contributor Author

ci-test

@riuvshin
Copy link
Contributor

ci-test build report:
Build details
Test report
selenium tests report data
docker image: eclipseche/che-server:10463
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@vkuznyetsov vkuznyetsov mentioned this pull request Jul 19, 2018
39 tasks
@mshaposhnik
Copy link
Contributor Author

ci-test

@riuvshin
Copy link
Contributor

ci-test build report:
Build details
Test report
selenium tests report data
docker image: eclipseche/che-server:10463
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@mshaposhnik
Copy link
Contributor Author

ci-test

@riuvshin
Copy link
Contributor

ci-test build report:
Build details
Test report
selenium tests report data
docker image: eclipseche/che-server:10463
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@Ohrimenko1988
Copy link
Contributor

@mshaposhnik Will this feature be enabled by default? I mean if it enabled we should rework tests or issue #10490 should be resolved previously.

@mshaposhnik
Copy link
Contributor Author

No it will be turned off by def.

@Ohrimenko1988
Copy link
Contributor

It looks like everything except problem which described in issue #10490 is OK. But the opinion of QE team is - the issue #10490 should be resolved before the merge this PR because we do not guarantee protection from regression. And if enable the "jwt-proxy" by default in future it will lead to big regression (20 %) and possibly additional problems which related to the absence of regression checking.

@Ohrimenko1988
Copy link
Contributor

Ohrimenko1988 commented Jul 23, 2018

Also a very important point. If this PR merge with master it also should be checked for regression with disabled "jwt-proxy".

@mshaposhnik
Copy link
Contributor Author

mshaposhnik commented Jul 24, 2018

@mshaposhnik
Copy link
Contributor Author

ci-test

@riuvshin
Copy link
Contributor

ci-test build report:
Build details
Test report
selenium tests report data
docker image: eclipseche/che-server:10463
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@mshaposhnik
Copy link
Contributor Author

ci-test

@riuvshin
Copy link
Contributor

ci-test build report:
Build details
Test report
selenium tests report data
docker image: eclipseche/che-server:10463
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@Ohrimenko1988
Copy link
Contributor

Tests which started on the assembly with disabled "jwt-proxy" didn't show any regression.
But the problem which described in the issue #10521 is still actual for enabled "jwt-proxy".
Disabling of this functionality looks like hiding the bug.
And it entails risks which I have described in the previous comment.

@mshaposhnik mshaposhnik merged commit 1471003 into master Jul 24, 2018
@mshaposhnik
Copy link
Contributor Author

Ok, merging now.

@mshaposhnik mshaposhnik deleted the exclusions branch July 24, 2018 15:18
@benoitf benoitf removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Jul 24, 2018
@benoitf benoitf added this to the 6.9.0 milestone Jul 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/task Internal things, technical debt, and to-do tasks to be performed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants